Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix LinkNeighborLoader producing double-sized edge_label_time for homogeneous graphs #7807

Merged
merged 16 commits into from
Jul 31, 2023

Conversation

akihironitta
Copy link
Member

@akihironitta akihironitta commented Jul 27, 2023

Fixes edge_label_time.size() == (2*batch_size,) to have (batch_size,).
Adds a test case for #7791.
Part of #7796 and #6528.

@github-actions github-actions bot removed the loader label Jul 27, 2023
@akihironitta akihironitta changed the title Add tests for temporal sampling in LinkNeighborLoader Fix LinkNeighborLoader producing double-sized edge_label_time for homogeneous graphs Jul 28, 2023

for sample in loader:
assert sample.edge_label_index.size() == (2, batch_size)
assert sample.edge_label_time.size() == (batch_size, )
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before this PR, the size of edge_label_time was (2*batch_size, ).

@akihironitta akihironitta marked this pull request as ready for review July 28, 2023 10:50
@akihironitta akihironitta requested review from wsad1, mananshah99 and a team as code owners July 28, 2023 10:50
@codecov
Copy link

codecov bot commented Jul 28, 2023

Codecov Report

Merging #7807 (ddd41d6) into master (bc69d1a) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head ddd41d6 differs from pull request most recent head 37287e7. Consider uploading reports for the commit 37287e7 to get more accurate results

@@           Coverage Diff           @@
##           master    #7807   +/-   ##
=======================================
  Coverage   91.57%   91.58%           
=======================================
  Files         455      455           
  Lines       25670    25656   -14     
=======================================
- Hits        23507    23496   -11     
+ Misses       2163     2160    -3     
Files Changed Coverage Δ
torch_geometric/sampler/neighbor_sampler.py 95.53% <100.00%> (+0.74%) ⬆️

... and 5 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Comment on lines 230 to 231
# TODO(akihironitta): Investigate the failure
# assert torch.all(sample.edge_time <= sample.edge_label_time)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could someone also confirm this assertion is correct before I dig into this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, will do.

Copy link
Member

@rusty1s rusty1s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I fixed the assert that was commented out. The issue was that time needs to be a node-level vector (for now), and that the endpoints of an edge needs to exist in time.

@rusty1s rusty1s enabled auto-merge (squash) July 31, 2023 14:41
@rusty1s rusty1s merged commit e8f752f into master Jul 31, 2023
@rusty1s rusty1s deleted the test/loader branch July 31, 2023 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants